Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Set shared=false for SortOrderBuilder #35848

Merged
merged 1 commit into from
Aug 31, 2022
Merged

Set shared=false for SortOrderBuilder #35848

merged 1 commit into from
Aug 31, 2022

Conversation

amenk
Copy link
Contributor

@amenk amenk commented Aug 1, 2022 •
edited by m2-assistant bot
Loading

Using a shared instance for this builder easily causes unwanted side effects

Description (*)

The builders do not need to be shared as per their definition. If a builder is shared, it can causes side effects.
While the actual effect was not reproducible on a vanilla Magento instance, the bug is quite obvious.

In our case the sorting of the customer grid in the admin panel did not have any effect, because the UiComponent building process interfered with \Magento\Customer\Model\GroupManagement::getLoggedInGroups

Manual testing scenarios (*)

I was not able to reproduce this on Vanilla Magento.

On our instance the repro steps were like this:

  1. Click title in custom grid to sort the list
  2. Does not have any effect

Questions or comments

I hope this does not go the good old "can reproduce" loop. The issue should be clear. Please merge.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

Resolved issues:

  1. resolves [Issue] Set shared=false for SortOrderBuilder #35939: Set shared=false for SortOrderBuilder

@amenk
Set shared=false for SortOrderBuilder
967f817 
Using a shared instance for this builder easily causes unwanted side effects
@m2-assistant
Copy link

m2-assistant bot commented Aug 1, 2022

Hi @amenk. Thank you for your contribution
Here are some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here

ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

@sdzhepa sdzhepa added the Priority: P3 May be fixed according to the position in the backlog. label Aug 4, 2022
@m2-community-project m2-community-project bot added this to Pending Review in Pull Requests Dashboard Aug 4, 2022
@ihor-sviziev
Copy link
Contributor

@sdzhepa I believe the priority should be P2 because you do not expect to have some state in the "Builder" classes, and this causes bugs in your code.

@m2-community-project m2-community-project bot moved this from Pending Review to Ready for Testing in Pull Requests Dashboard Aug 5, 2022
@ihor-sviziev ihor-sviziev added Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests Progress: pending review labels Aug 5, 2022
@m2-community-project m2-community-project bot moved this from Ready for Testing to Pending Review in Pull Requests Dashboard Aug 5, 2022
@m2-community-project m2-community-project bot moved this from Pending Review to Ready for Testing in Pull Requests Dashboard Aug 5, 2022
@ihor-sviziev ihor-sviziev added Progress: ready for testing Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests and removed Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests labels Aug 5, 2022
@ihor-sviziev
Copy link
Contributor

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

Den4ik
Den4ik approved these changes Aug 5, 2022
View reviewed changes
Copy link
Contributor

@Den4ik Den4ik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @amenk Thanks for your contribution.
I'm agree that this class should not be shared due to possible mistakes if create method don't called.
I don't see any blockers for merge this PR

@amenk
Copy link
Contributor Author

amenk commented Aug 5, 2022

Awesome, thanks :-)

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@ihor-sviziev ihor-sviziev added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. and removed Priority: P3 May be fixed according to the position in the backlog. labels Aug 5, 2022
@engcom-Lima engcom-Lima self-assigned this Aug 12, 2022
@engcom-Lima engcom-Lima moved this from Ready for Testing to Testing in Progress in High Priority Pull Requests Dashboard Aug 12, 2022
@engcom-Lima
Copy link
Contributor

@magento give me 2.4-develop instance

@magento-deployment-service
Copy link

Hi @engcom-Lima. Thank you for your request. I'm working on Magento instance for you.

@engcom-Lima
Copy link
Contributor

@magento create issue

@m2-assistant m2-assistant bot mentioned this pull request Aug 12, 2022
Closed
5 tasks
@m2-community-project m2-community-project bot moved this from Testing in Progress to Ready for Testing in High Priority Pull Requests Dashboard Aug 12, 2022
@magento-deployment-service
Copy link

Hi @engcom-Lima, here is your Magento Instance: https://5904f9752fe1ed099eeeee633d357de1.instances.magento-community.engineering
Admin access: https://5904f9752fe1ed099eeeee633d357de1.instances.magento-community.engineering/admin_7570
Login: 8695200c
Password: f71c08c662bd

@engcom-Lima engcom-Lima moved this from Ready for Testing to Testing in Progress in High Priority Pull Requests Dashboard Aug 12, 2022
@engcom-Lima
Copy link
Contributor

HI @amenk ,

Thanks for your contribution and collaboration.

I have tried to reproduce the issue but issue is not reproducible to me on 2.4-develop.
I have followed below steps to reproduce:

  1. Login as admin.
  2. Click on Customers >> Customer groups.
  3. Sorted the title.

I am able to sort the title successfully. Please verify my video.

Kindly provide me more information in order to reproduce the issue.

Thanks

@amenk
Copy link
Contributor Author

amenk commented Aug 12, 2022

@engcom-Lima Thanks for testing, but it does not make sense too me.
I wrote it is not reproducible in Vanilla Magento but can be triggered by customization. The fix is pretty clear, so please skip the reproducing-procedure here and merge it, so other developers do not waste countless hours of debugging as well for such a simple fix. Thanks :-)

@engcom-Alfa
Copy link
Contributor

engcom-Alfa commented Aug 18, 2022 •
edited
Loading

✔️ QA Passed

As we tried, the issue is not reproducible in the 2.4-develop Magento clean instance locally.
As discussed about the file changes with the internal developers, the changed code has no impact on any feature.
Hence, taken the PR changes into the 2.4-develop and regressed the UI admin table reflection.

  1. Checked in the admin>Customer>All Customers table. (By having ~20 customer data).
  2. Checked in the Reports tables.
  3. Checked in the Content>Pages table.
  4. Checked in the Catalog>Products table.

As no regression cause noticed, forwarding the ticket to MIP

@engcom-Alfa engcom-Alfa self-assigned this Aug 18, 2022
@m2-community-project m2-community-project bot moved this from Testing in Progress to Ready for Testing in High Priority Pull Requests Dashboard Aug 18, 2022
@engcom-Alfa engcom-Alfa moved this from Ready for Testing to Testing in Progress in High Priority Pull Requests Dashboard Aug 18, 2022
@engcom-Alfa engcom-Alfa moved this from Testing in Progress to Merge in Progress in High Priority Pull Requests Dashboard Aug 18, 2022
@magento-devops-reposync-svc magento-devops-reposync-svc merged commit d698d8b into magento:2.4-develop Aug 31, 2022
@ihor-sviziev ihor-sviziev moved this from Merge in Progress to Recently Merged in High Priority Pull Requests Dashboard Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Den4ik Den4ik Den4ik approved these changes

@ihor-sviziev ihor-sviziev ihor-sviziev approved these changes

Assignees

@Den4ik Den4ik

@ihor-sviziev ihor-sviziev

@engcom-Alfa engcom-Alfa

@engcom-Lima engcom-Lima

Labels
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: accept Risk: low Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Issue] Set shared=false for SortOrderBuilder
7 participants
@amenk @ihor-sviziev @engcom-Lima @engcom-Alfa @Den4ik @sdzhepa @magento-devops-reposync-svc